Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix compilation errors in clang 17 #117

Closed
wants to merge 1 commit into from

Conversation

Destroyerrrocket
Copy link
Contributor

No description provided.

@Destroyerrrocket
Copy link
Contributor Author

This addresses #115 and #111

@VelocityRa
Copy link
Contributor

VelocityRa commented Jun 24, 2024

Could you replace <cstdint> with stdint.h as described on my issue? I had to do that when compiling via libclang (18). cstdint isn't guaranteed to export these types other than in std::.

@Destroyerrrocket
Copy link
Contributor Author

Does your distribution not ship the global definitions in the cstdint header? Could I get some details on your setup?
If you mean it in the "this is not necessarily guaranteed but works 100% of the time" kind of thing, then I think I'll let the maintainer of this repo make the call

@VelocityRa
Copy link
Contributor

VelocityRa commented Jun 24, 2024

I'm using a fork of https://github.com/pybind/pybind11_mkdoc on Ubuntu 24.04 with clang-tools-18 installed from apt.

I get errors about uint8_t not being known unless I patch the call to libclang to additionally pass -include stdint.h (which implicitly includes that header everywhere).

@Destroyerrrocket
Copy link
Contributor Author

Destroyerrrocket commented Jun 24, 2024

Interesting, I'll see if I can reproduce this, it's the first time I see a distribution make this dumb of a decision! In any case, consider it done (tomorrow probably)!

@VelocityRa
Copy link
Contributor

VelocityRa commented Jul 21, 2024

Bump?

@fraillt Hi, is this repo dead?

@fraillt
Copy link
Owner

fraillt commented Jul 21, 2024

Thanks for pinging ;)
It's not dead, I just consider it feature complete, so I don't actively work on it anymore, but small improvements, like yours, are always welcome;)

@Destroyerrrocket
Copy link
Contributor Author

Hi! Sorry, you just caught me on vacation, I committed the change from cstdint to stdint.h, if you can resolve the small conflict and squash the changes, that would be ideal, if not I'll do it when I have a sec.

I completely get that if there's nothing to improve, there's simply nothing to change! I have a question, would you be open to getting C++23 code for flat_maps and stuff like that? I imagine that at some point we'll start using it and will want to be able to serialize it.

@fraillt
Copy link
Owner

fraillt commented Jul 21, 2024

I wouldn't mind to accept these changes as part of extensions, there already exists some extensions for variant and optional which both are C++17. I'm a bit afraid to introduce new C++ standard to core bitsery functionality...

Btw, at the moment, I don't have access to my computer either, maybe at the end of next week I'll be able to sit down and resolve conflicts..

@VelocityRa
Copy link
Contributor

There don't seem like to be any conflicts? CI failure is unrelated and on master too.
And squashing can be done from the GitHub UI when merging by @fraillt.

@Destroyerrrocket
Copy link
Contributor Author

Destroyerrrocket commented Jul 21, 2024

I wouldn't mind to accept these changes as part of extensions, there already exists some extensions for variant and optional which both are C++17. I'm a bit afraid to introduce new C++ standard to core bitsery functionality...

Absolutely no need to use them in the main core functionality, I was just thinking of adding extensions for those, as you say. It can all be disabled based on what C++ version you're using to compile with.

I think it is fine and very reasonable for bitsery to support C++11, as looking at the last surveys by the committee (which will have selection bias) still show that more than 25% of users still can't fully use C++17...

Btw, at the moment, I don't have access to my computer either, maybe at the end of next week I'll be able to sit down and resolve conflicts..

That's fine, I'm not in a rush! I have a bit of time, let me see if I can rebase stuff...

And remove maybe_unused as that is a C++17 extension and could
potentially not work in its job of silencing an "unused" warning.
@Destroyerrrocket
Copy link
Contributor Author

Destroyerrrocket commented Jul 21, 2024

@VelocityRa I had made a quick merge from my phone, previously it complained.
Please check that it indeed now uses stdint.h headers

@VelocityRa
Copy link
Contributor

@Destroyerrrocket Looks good, thank you!

@VelocityRa
Copy link
Contributor

Bump?

@fraillt
Copy link
Owner

fraillt commented Jul 26, 2024

Help me to understand this change... the way I understand, stdint.h file from C standart library (C99), while cstdint> file is from C++ standart library (C+11).
My first thought is that cstdint should be preferred.
Maybe the issue is with your LLVM configuration? https://stackoverflow.com/questions/76798324/error-with-include-cstdint-using-clang-17

On the other hand, maybe this is exactly what you need (use LLVM without C++ std library)?

@Destroyerrrocket
Copy link
Contributor Author

The C++ standard technically only guarantees fixed size ints to be on the std namespace with the cstdint header, while the C header that is kept for retrocompatibility only guarantees the symbols on the global namespace. In practice, I have not ever seen a single case where the cstdint header didn't also include the global symbols, but @VelocityRa insisted on keeping the C headers. I believe the issue you linked is the most likely cause of what he reported, so I think that this change can be reverted. There's also the point of the maybe_unused attribute being used, which technically could be a problem as it is not included by default in C++11 (but most implementations have support for more modern attributes in older standards)

@VelocityRa
Copy link
Contributor

VelocityRa commented Jul 26, 2024

That link seems unrelated and I do compile with STD as you normally would.

The errors were:

vcpkg_installed/x64-linux/include/bitsery/details/adapter_common.h:54:13: error: unknown type name 'uint8_t'
etc
vcpkg_installed/x64-linux/include/bitsery/details/adapter_common.h:138:62: error: use of undeclared identifier 'uint8_t'

I also got this issue on GCC 13 a few days ago:

vcpkg_installed/x64-linux/include/bitsery/details/adapter_common.h: In function ‘void bitsery::details::readSize(Reader&, size_t&, size_t, TCheckMaxSize)’:
vcpkg_installed/x64-linux/include/bitsery/details/adapter_common.h:54:13: error: ‘uint8_t’ was not declared in this scope
etc
vcpkg_installed/x64-linux/include/bitsery/details/adapter_common.h:36:1: note: ‘uint8_t’ is defined in header ‘<cstdint>’; did you forget to ‘#include <cstdint>’?
etc

So it's not just a clang/llvm issue.

And including stdint.h fixes it (I just do it by passing -include stdint.h to the compiler currently, which is an ugly workaround).

@Destroyerrrocket
Copy link
Contributor Author

I know that this will be annoying, but could it be possible for you to test the latest version of the repo? I needed to include the cstdint headers and that was merged earlier.

This way we can absolutely be sure that this is indeed needed. Also, if you could provide the output from make with the -n flag so it gives the compiler command being used where it fails, we could maybe get some extra info on this odd setup. If this still happens, I will take action to actually reproduce this because I simply find it really really hard to believe that Ubuntu specifically would choose to be pedantic and not include the global definitions for the obscure reason that the standard does not guarantee it.

Also, I'd try to reinstall the standard library you're using just to be safe.

Sorry if I'm asking for a lot of actions...

@fraillt
Copy link
Owner

fraillt commented Jul 26, 2024

I did some testing using Docker.
There's the script that I made.

FROM ubuntu:latest
RUN apt update
RUN apt install git cmake libgmock-dev -y
WORKDIR /testing
RUN git clone https://github.com/fraillt/bitsery.git
WORKDIR bitsery
RUN apt install clang-17 -y
RUN update-alternatives --install /usr/bin/c++ c++ /usr/bin/clang++-17 100
RUN update-alternatives --install /usr/bin/cc cc /usr/bin/clang-17 100
RUN cmake -S . -B build -DBITSERY_BUILD_TESTS=ON -DBITSERY_BUILD_EXAMPLES=ON
RUN cmake --build build
RUN ctest --test-dir build

Put this in dockerfile and run with docker build -t test-bitsery .

The outcome is that it works just fine with clang-17 using bitsery master branch with no warnings...

@VelocityRa
Copy link
Contributor

VelocityRa commented Jul 28, 2024

Oh I just saw https://github.com/fraillt/bitsery/pull/106/files... it was merged a year ago but the latest release (v5.2.3) doesn't include it.
I don't use master, I use the vcpkg port which uses tagged releases.

I can't test right now but that's most likely the cause. It also explains why -include stdint.h fixed it (cstdint would fix it too).

@fraillt After this PR (with the stdint.h change removed by @Destroyerrrocket) is merged, could you make a new release, please?

@fraillt
Copy link
Owner

fraillt commented Jul 29, 2024

I did prepared 5.2.4 in development branch.
I plan to release it tomorrow.

@fraillt
Copy link
Owner

fraillt commented Jul 30, 2024

I hope new release fixes all the problems.

@fraillt fraillt closed this Jul 30, 2024
@VelocityRa
Copy link
Contributor

Also updated the vcpkg port: microsoft/vcpkg#40177
Thank you both :)

@Destroyerrrocket
Copy link
Contributor Author

Thank you so much for the work, @fraillt ! ^-^

Would it be possible for you to update the Conan version? If it's too much work, maybe I can do that myself

@fraillt
Copy link
Owner

fraillt commented Jul 31, 2024

I'm not familiar with Conan, maybe you could do it? :)
I know that there exist Conan package for bitsery, but I'm not sure who was that nice person that made this happen :)

@Destroyerrrocket
Copy link
Contributor Author

I see! In any case, thank you for maintaining this great library

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants